Skip to content

fix(ingestion-pipeline): inherit owners from service and authorize trigger#28109

Open
manerow wants to merge 6 commits into
mainfrom
fix/27962-ingestion-pipeline-owner-inheritance
Open

fix(ingestion-pipeline): inherit owners from service and authorize trigger#28109
manerow wants to merge 6 commits into
mainfrom
fix/27962-ingestion-pipeline-owner-inheritance

Conversation

@manerow
Copy link
Copy Markdown
Contributor

@manerow manerow commented May 14, 2026

Fixes #27962

Summary

Two coordinated platform fixes to make isOwner()-based authorization work correctly on IngestionPipeline and to close a long-standing gap on the trigger endpoint:

  1. Owner inheritanceIngestionPipeline.owners now inherits from the parent referenced by service (Service / TestSuite / App). Inherited owners are tagged inherited: true, indexed for search, and visible to the policy evaluator. Test-suite pipelines inherit transitively from the underlying Table via TestSuiteRepository.setInheritedFields.
  2. Trigger authorizationPOST /v1/services/ingestionPipelines/trigger/{id} previously skipped authorizer.authorize(...) entirely, so any authenticated user could trigger any pipeline. It now authorizes against MetadataOperation.TRIGGER (the existing enum value already declared in the method for limits.enforceLimits — we just wire it to the authorizer).

A coordinated set of seeded-policy updates + idempotent upgrade migration preserves pre-fix behavior for default roles that previously had broad capability, so no customer is surprised by the new authz check on the trigger endpoint.

What's changing

1. IngestionPipelineRepository.setInheritedFields override

@Override
public void setInheritedFields(IngestionPipeline ingestionPipeline, Fields fields) {
  EntityReference serviceRef = ingestionPipeline.getService();
  if (serviceRef == null) return;
  try {
    EntityInterface parent = Entity.getEntity(serviceRef, "owners,domains", ALL);
    inheritOwners(ingestionPipeline, fields, parent);
    inheritDomains(ingestionPipeline, fields, parent);
  } catch (EntityNotFoundException e) {
    LOG.debug("Parent service {} not found for ingestion pipeline {}; skipping inheritance",
        serviceRef.getFullyQualifiedName(), ingestionPipeline.getFullyQualifiedName());
  }
}
  • Mirrors the established pattern (TableRepository.setInheritedFields:311-321, TestSuiteRepository.setInheritedFields, etc.).
  • Uses the existing inheritOwners / inheritDomains helpers (EntityRepository.java:4510-4525), which already null-guard and tag refs with inherited=true.
  • try/catch EntityNotFoundException protects GET / list endpoints from 500s when a service is soft-deleted with orphan pipelines.
  • setFieldsInBulk already routes through super.setFieldsInBulk → base setInheritedFields(List) → per-entity call, so list endpoints get inheritance for free. No bulk override needed.

2. IngestionPipelineResource.triggerPipelineInternal authorize call

Adds the missing authorizer.authorize(...) call to the existing OperationContext(entityType, MetadataOperation.TRIGGER) that was already built in the method:

public PipelineServiceClientResponse triggerPipelineInternal(
    UUID id, UriInfo uriInfo, SecurityContext securityContext, String botName) {
  OperationContext operationContext = new OperationContext(entityType, MetadataOperation.TRIGGER);
  authorizer.authorize(securityContext, operationContext, getResourceContextById(id));
  if (pipelineServiceClient == null) { ... }
  // ... rest unchanged, including limits.enforceLimits(...)
}
  • Placed before the pipelineServiceClient == null early-return so authz fires even in environments without an Airflow client (tests, dev, customer instances without pipeline service).
  • Uses the existing TRIGGER operation value rather than EDIT_ALL, consistent with upstream AI #200 - Add TRIGGER permission to application bots #25113 (AI #200 - Add TRIGGER permission to application bots) which already established Trigger as a discrete, explicitly-granted operation.
  • getResourceContextById(id) builds a ResourceContext that loads owners for authz, so the inherited owners populated by Change 1 are visible to isOwner() evaluation.

3. Seeded policies updated for Trigger + upgrade migration

EditAll does not imply Trigger (per CompiledRule.matchOperation:221-224 — only Edit*-prefixed ops are covered by EditAll). To preserve pre-fix behavior for default roles that previously had broad capability, six seeded policies receive Trigger via an idempotent migration:

Policy file Rule Why
IngestionBotPolicy IngestionBotRule-Allow Broad bot — main automation identity
LineageBotPolicy LineageBotRule-Allow Broad bot on ["All"] resources
ProfilerBotPolicy ProfilerBotBotRule-Allow Broad bot on ["All"] resources
QualityBotPolicy QualityBotBotRule-Allow Broad bot on ["All"] resources
UsageBotPolicy UsageBotRule-Allow-Usage Broad bot on ["All"] resources
DataStewardPolicy DataStewardPolicy-EditRule Has EditOwners on all resources — stewards could already reach trigger via an ownership-rewrite escalation (verified empirically). Granting Trigger directly aligns the policy with the effective capability and improves audit clarity.

Migration (migration/utils/v1129/MigrationUtil.addTriggerOperationToDefaultPolicies) iterates the six (policy, rule) targets and calls the existing idempotent v160.MigrationUtil.addOperationsToPolicyRule helper. Wired into:

  • migration/{mysql,postgres}/v1129/Migration.java — runs on upgrade to 1.12.9 (companion empty SQL placeholders in bootstrap/sql/migrations/native/1.12.9/{mysql,postgres}/).
  • migration/{mysql,postgres}/v1130/Migration.java — runs again on upgrade to 1.13.0 as an idempotent safety net (existing runDataMigration body extended with the call).

Deliberately out of scope:

  • AutoClassificationBotPolicyEditAll is scoped to Table / Container only; Trigger doesn't apply to those resources, so adding it would be a no-op.
  • DataConsumerPolicy — no EditAll, no EditOwners; Data Consumers genuinely shouldn't trigger pipelines.
  • OrganizationPolicy-NoOwner-Rule — adding Trigger here would re-introduce trivial-trigger for any unowned entity via noOwner().
  • OrganizationPolicy-Owner-Rule already grants [All] (which includes Trigger) to isOwner(), so no change needed.
  • CompiledRule.matchOperationno platform-wide semantic change. EditAll → Trigger would have been a hard rule on the operator. We modify shipped policies, not the operator's behavior, so customer-built EditAll policies remain unchanged and customers retain control over which roles trigger.

4. Tests

New file: openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IngestionPipelineOwnerInheritanceIT.java. Two tests in the new SDK-client framework (the old IngestionPipelineResourceTest.java was removed in #26204):

  1. testInheritedOwners_fromService — service owner appears on pipeline GET with inherited: true.
  2. testIsOwnerPolicy_appliesToEditAndTrigger — full Policy → Role → User chain with Operations: [EditAll, Trigger], Condition: isOwner(). Owner can PATCH displayName + POST /trigger; non-owner gets precise permissionNotAllowed(USER2, [EditDisplayName]) / permissionNotAllowed(USER2, [Trigger]) 403s.

Behavior change to call out

/trigger now requires Trigger. Three groups of identities are affected:

Can trigger after the fix:

  • Admins (unchanged — via All).
  • Owners of the parent Service / TestSuite / App, via the default OrganizationPolicy-Owner-Rule. Inheritance fix makes this fire automatically.
  • Users with the default DataSteward role (via the migration above).
  • All seeded bots that previously had broad EditAll: Ingestion, Application, Lineage, Profiler, Quality, Usage.
  • Anyone granted Trigger explicitly in a custom policy.

Cannot trigger after the fix (intentional tightening):

  • Default DataConsumer role.
  • AutoClassificationBot (its EditAll scope doesn't cover ingestionPipeline).
  • DefaultBot / ScimBot (no broad edit grant).
  • Generic authenticated users with no ownership and no Trigger grant — closes the pre-fix trivial-trigger hole.

Customers should audit (the narrow regression surface):

  • Custom automation that calls /trigger using a non-admin / non-default-bot identity needs an explicit Trigger grant.
  • Custom-built roles with broad EditAll on ingestionPipeline (not the default DataSteward, which is covered by the migration) need Trigger added to keep their pre-fix trigger capability.

Unaffected:

  • Scheduled Airflow runs — the scheduler fires DAGs directly; the metadata CLI only does GET /pipelineStatus and PUT /pipelineStatus, neither of which calls /trigger.

Why this shape

  • Trigger remains discrete — consistent with upstream AI #200 - Add TRIGGER permission to application bots #25113. EditAll is the "broad edit capability" operator and Trigger is an "action" operation (alongside Deploy, Kill). Conflating them would weaken the operation model and was rejected in favor of explicit seeded-policy grants.
  • DataStewardPolicy gets Trigger directly rather than relying on the ownership-edit escalation. The capability is functionally already there for stewards; making it explicit removes the indirection and gives a cleaner audit trail (one Trigger event vs. an EditOwners PATCH followed by a Trigger).
  • Seeded policies are modified via data, not via operator semantics so customer-built EditAll policies retain customer-controlled semantics. Customers who want broader trigger access add Trigger to their own roles; customers who want stricter trigger gates (like Orsted) get exactly what their isOwner() policy already expressed.

Test plan

  • IngestionPipelineOwnerInheritanceIT#testInheritedOwners_fromService passes.
  • IngestionPipelineOwnerInheritanceIT#testIsOwnerPolicy_appliesToEditAndTrigger passes.
  • Full mvn -pl openmetadata-integration-tests -am clean install -DskipTests builds cleanly.
  • mvn spotless:apply clean.
  • Local upgrade smoke test: brought up a fresh 1.12.9 stack, confirmed Trigger present in IngestionBotPolicy / LineageBotPolicy / ProfilerBotPolicy / QualityBotPolicy / UsageBotPolicy / DataStewardPolicy post-migration via GET /api/v1/policies/name/{name}?fields=rules.
  • Six API scenarios validated end-to-end against a deployed stack with metadata-ingested data (TestSuite pipeline, Metadata pipeline, Profiler pipeline, default-Owner-Rule, IngestionBot regression check, admin sanity).
  • Playwright UI specs cover owner-can / non-owner-denied / inherited-owner across the agents tab.
  • Reindex ingestion_pipeline_search_index after deploy so the UI owner filter sees inherited owners.

Summary by Gitar

  • Refactored log storage:
    • Made closeStream idempotent by returning early if log storage is not configured instead of throwing an IllegalStateException.

This will update automatically on new commits.

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels May 14, 2026
Comment thread ingestion/src/metadata/ingestion/source/dashboard/looker/utils.py Fixed
Comment thread ingestion/src/metadata/ingestion/source/dashboard/looker/utils.py Fixed
@manerow manerow force-pushed the fix/27962-ingestion-pipeline-owner-inheritance branch from a76c6e2 to a3d7299 Compare May 14, 2026 10:13
@manerow manerow force-pushed the fix/27962-ingestion-pipeline-owner-inheritance branch from a3d7299 to 057d7d0 Compare May 14, 2026 12:22
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

🟡 Playwright Results — all passed (17 flaky)

✅ 4063 passed · ❌ 0 failed · 🟡 17 flaky · ⏭️ 92 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 297 0 2 4
🟡 Shard 2 754 0 8 14
🟡 Shard 3 779 0 2 7
🟡 Shard 4 788 0 2 18
🟡 Shard 5 708 0 1 41
🟡 Shard 6 737 0 2 8
🟡 17 flaky test(s) (passed on retry)
  • Features/MetricCustomUnitFlow.spec.ts › Should create metric and test unit of measurement updates (shard 1, 1 retry)
  • Features/Pagination.spec.ts › should test pagination on Observability Alerts page (shard 1, 1 retry)
  • Features/ActivityAPI.spec.ts › creates an activity event when tags are added (shard 2, 1 retry)
  • Features/Glossary/GlossaryHierarchy.spec.ts › should move term with children to different glossary (shard 2, 1 retry)
  • Features/Glossary/GlossaryWorkflow.spec.ts › should start term as Draft when glossary has reviewers (shard 2, 1 retry)
  • Features/KnowledgeCenterList.spec.ts › Knowledge Center List - Test infinite scroll/pagination (shard 2, 1 retry)
  • Features/KnowledgeCenterList.spec.ts › Knowledge Center List - Test add article button (shard 2, 1 retry)
  • Features/KnowledgeCenterTextEditor.spec.ts › Rich Text Editor - Text Formatting (shard 2, 1 retry)
  • Features/KnowledgeCenterTextEditor.spec.ts › Rich Text Editor - Text Formatting (shard 2, 1 retry)
  • Features/KnowledgeCenterTextEditor.spec.ts › Rich Text Editor - Text Formatting (shard 2, 1 retry)
  • Features/RTL.spec.ts › Verify Following widget functionality (shard 3, 1 retry)
  • Flow/ServiceForm.spec.ts › Verify form selects are working properly (shard 3, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Table (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate DataProduct Rule Any_In (shard 4, 1 retry)
  • Pages/ExplorePageRightPanel_KnowledgeCenter.spec.ts › Should remove user owner for knowledgeCenter (shard 5, 1 retry)
  • Pages/GlossaryImportExport.spec.ts › Glossary CSV import preserves typed relations (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented May 15, 2026

Code Review ✅ Approved 5 resolved / 5 findings

Enables ingestion pipeline owner inheritance from parent services and enforces mandatory authorization for trigger operations. All identified issues, including legacy test dependencies and policy configuration gaps, have been resolved.

✅ 5 resolved
Performance: batchFetchServices filter misses non-service parents

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/IngestionPipelineRepository.java:248
In batchFetchServices, the filter fromEntity.endsWith("_service") will not match testSuite or app entity types that can also be the parent (container) of an IngestionPipeline. This causes test-suite and application pipelines to always fall through to the individual getContainer() fallback on line 200, defeating the purpose of the batch optimization for those pipeline types.

The impact is minor since the fallback is correct and these pipeline types are typically fewer in number, but it's worth noting for completeness of the batch optimization.

Quality: Integration test uses legacy Joda-Time instead of java.time

📄 openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IngestionPipelineOwnerInheritanceIT.java:12 📄 openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IngestionPipelineOwnerInheritanceIT.java:57
The new integration test imports org.joda.time.DateTime (line 12) to construct a Date object. Per project standards (Java 21 features), this should use java.time APIs instead. Joda-Time is a legacy library superseded by java.time since Java 8.

Quality: PR description contradicts code: LineageBot DOES get Trigger

📄 openmetadata-service/src/main/java/org/openmetadata/service/migration/utils/v1129/MigrationUtil.java:30-37 📄 openmetadata-service/src/main/resources/json/data/policy/LineageBotPolicy.json:21
The PR summary states "Other system bots (LineageBot, ProfilerBot, UsageBotBot, ...) do not have Trigger" but the code explicitly adds Trigger to LineageBotPolicy.json, ProfilerBotPolicy.json, QualityBotPolicy.json, and UsageBotPolicy.json — both in the seed files and in the migration utility (MigrationUtil.addTriggerOperationToDefaultPolicies). This documentation mismatch could confuse reviewers and future maintainers about the intended security posture. Please update the PR description's "Behavior change to call out" section to accurately reflect that all bot policies receive Trigger.

Quality: Generic catch swallows migration failures silently

📄 openmetadata-service/src/main/java/org/openmetadata/service/migration/utils/v1129/MigrationUtil.java:90-92
Line 90 catches Exception broadly and only logs the error. If the DAO update fails (e.g., serialization issue, constraint violation), the migration will report success while the DataStewardPolicy remains un-patched. The bot-policy counterpart (addTriggerOperationToDefaultBotPolicies via the v160 helper) propagates exceptions, causing the migration to fail visibly.

For a data migration, failing loudly is preferable so operators notice and retry, rather than silently leaving an incomplete policy state that's hard to diagnose later.

Quality: DataStewardPolicy.json missing trailing newline

📄 openmetadata-service/src/main/resources/json/data/policy/DataStewardPolicy.json:23
The file ends without a trailing newline (\ No newline at end of file in the diff). While cosmetic, this causes noisy diffs when a future change appends content and most linters/editors flag it.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ownership-based access control is not working for Test Suite Ingestion Pipelines

5 participants